Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add major selection to profile #501

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Conversation

mjaydenkim
Copy link
Contributor

Summary

This PR works on a major update... haha i'm so funny...... anyway it allows users to set their major as a part of their account rather than each review individually (although reviews stay attached to the major selected at the time of publication).

New users are prompted to select their major on the review page with the same UI as before, but their selection will then attach to their account. Users shouldn't be shown the major selection dropdown otherwise as long as they continue to have a selected major and are logged in. Existing users can update their major on the profile page, where there's a dropdown menu to do so.

PR Type

  • 🍕 Feature
  • 🐛 Bug Fix
  • 📝 Documentation Update
  • 🎨 Style
  • 🧑‍💻 Code Refactor
  • 🔥 Performance Improvements
  • ✅ Test
  • 🤖 Build
  • 🔁 CI
  • 📦 Chore (Release)
  • ⏩ Revert

Mobile + Desktop Screenshots & Recordings

https://github.com/user-attachments/assets/88ed0c9b-105f-4a37-a8fc-bf0b5bad2a08
https://github.com/user-attachments/assets/8c0ca687-3eb0-4a21-aa40-6be17db0dfa1
majorupdate_ss1

QA - Test Plan

Tested in a variety of different account scenarios, including with accounts that have no major, accounts with many majors, etc. Tested new UIs on mobile, but there isn't a ton that's different. Couldn't test with a new account (b/c I don't have other NetIDs) but I simulated that performance essentially. Also tested to ensure that majors are still correctly tied to reviews.

Breaking Changes & Notes

  • All logged-out users are shown the major dropdown, and selecting a new major for an existing user notifies that user (via toast) and changes the major to the more recent selection.

Added to documentation?

  • 📜 README.md
  • 📓 notion
  • 🍕 ...
  • 📕 ...
  • 🙅 no documentation needed

What GIF represents this PR?

gif

Need to style, test. Also if major is being overwritten b/c a user was logged out, add a confirmation modal
Also got rid of a console log lol
@mjaydenkim mjaydenkim requested a review from a team as a code owner January 17, 2025 08:29
@dti-github-bot
Copy link
Member

dti-github-bot commented Jan 17, 2025

[diff-counting] Significant lines: 293.

Copy link
Contributor

@leihelen leihelen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really good and will be a really important feature since a lot of people are turned away from posting reviews because there are so many steps to complete so this will help simplify the process! I tested the same scenarios you went through and it works well on my end as well. Maybe you could add some try catch statements when you call the api and make post requests in case of unexpected errors as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants